Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable markdownlint rule (MD009) #40887

Merged
merged 7 commits into from
Feb 8, 2021
Merged

Conversation

Youssef1313
Copy link
Member

Related to #40884

cc: @jkotas @stephentoub there are too much violations of this rule in markdown files. To know if there are significant whitespaces or not, we will need to fix all the violations, and check if the diff shows any visual changes. Do you want me to fix that?

You can view the violations from here.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Aug 15, 2020

Yes, fixing the end of line whitespaces in the markdown files is welcomed. I would do it in a separate PR from the infrastructure changes to enable markdown lint.

@ghost
Copy link

ghost commented Aug 15, 2020

Tagging subscribers to this area: @ViktorHofer
See info in area-owners.md if you want to be subscribed.

monojenkins pushed a commit to monojenkins/mono that referenced this pull request Aug 20, 2020
akoeplinger pushed a commit to mono/mono that referenced this pull request Aug 20, 2020
Related to dotnet/runtime#40887 and dotnet/runtime#40884

Co-authored-by: Youssef1313 <Youssef1313@users.noreply.github.com>
@stephentoub
Copy link
Member

@Youssef1313, is this PR still relevant at this point, or should it be closed?

@Youssef1313
Copy link
Member Author

Should be closed I think given that the repo doesn't allow GitHub Actions.

@Youssef1313 Youssef1313 deleted the patch-12 branch September 14, 2020 16:01
@ViktorHofer
Copy link
Member

@Youssef1313 We now use at least one github action in dotnet/runtime so feel free to reopen. We should be able to get this in.

@Youssef1313 Youssef1313 restored the patch-12 branch September 14, 2020 16:58
@Youssef1313 Youssef1313 reopened this Sep 14, 2020
@Youssef1313
Copy link
Member Author

@Youssef1313 We now use at least one github action in dotnet/runtime so feel free to reopen. We should be able to get this in.

That's great!

@jkotas
Copy link
Member

jkotas commented Sep 14, 2020

@Youssef1313
Copy link
Member Author

@jkotas I'll delete that section and fix the new markdown violations.

@ghost
Copy link

ghost commented Feb 5, 2021

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

@danmoseley danmoseley closed this Feb 5, 2021
@danmoseley danmoseley reopened this Feb 5, 2021
@ghost
Copy link

ghost commented Feb 5, 2021

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

As advised by @ViktorHofer so it's quite clear it only runs in CI.
@ghost
Copy link

ghost commented Feb 5, 2021

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

@danmoseley
Copy link
Member

summarizing discussion with @ViktorHofer - I removed the "push" section, so that this only runs on PR's, and does not trigger a Node install on rolling builds/CI. Our official builds, which produce shipping bits, do not run in Github pipelines, so this should not trigger Node there, regardless.

@ghost
Copy link

ghost commented Feb 8, 2021

Apologies, I am afraid I am encountering technical difficulties that might have hampered my ability to assist with merging this pull request. I will continue to try to assist if there are further changes to this pull request.

@danmoseley danmoseley merged commit 5d5c3e7 into dotnet:master Feb 8, 2021
@danmoseley
Copy link
Member

Thanks for the contribution @Youssef1313

@Youssef1313 Youssef1313 deleted the patch-12 branch February 8, 2021 18:58
@BruceForstall
Copy link
Member

@danmosemsft Does this mean that changes to markdown files can lead to PR test failures? That seems extremely unnecessary.

@danmoseley
Copy link
Member

danmoseley commented Feb 9, 2021

@BruceForstall In retrospect I agree (my focus was on just getting this PR finished as I observed it was old). I think this step is low value, although perhaps more rules could be enabled like checking for broken links.

@safern is looking to see whether we can make this rule only trigger for markdown changes -- then if someone somehow checks in bad markdown, it won't affect 99% of PR's. Is that reasonable?

@Youssef1313
Copy link
Member Author

@safern is looking to see whether we can make this rule only trigger for markdown changes

This is already the case.
This workflow is only run when there are changes in md files.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Feb 9, 2021

@danmosemsft Does this mean that changes to markdown files can lead to PR test failures? That seems extremely unnecessary.

Only if the PR has bad markdown changes only.

@danmoseley
Copy link
Member

Also note that if it does fail on a markdown edit, it fails within moments of pushing to the PR, so you don't waste an hour or two waiting for the jobs to complete.

As mentioned, my care level about spacing is low, but there are more interesting rules (I have hand-fixed many broken links in the past and would rather not)

@BruceForstall
Copy link
Member

@safern is looking to see whether we can make this rule only trigger for markdown changes -- then if someone somehow checks in bad markdown, it won't affect 99% of PR's. Is that reasonable?

It would have no effect, since if you don't PR markdown and the rule doesn't fire, it won't have anything to complain about. Anything with markdown will be subject to the rule.

Your broken link example would be useful, but what if a link went away (even transiently) and my unrelated markdown change got flagged (well, assuming the rule applied to all markdown, not just the changed one). That would be annoying. And if it doesn't check all existing md for missing links, then there's no value anyway.

The question is whether we want to validate markdown at all. I hope we encourage markdown / documentation writing and don't discourage it in any way -- don't make it subject to the vagaries / flakiness of the CI system, for example, or make me wait for the CI to complete before merging.

One improvement would be if a markdown-only change only did markdown testing and nothing else.

@danmoseley
Copy link
Member

It would have no effect, since if you don't PR markdown and the rule doesn't fire, it won't have anything to complain about. Anything with markdown will be subject to the rule.

The case I was concerned about was where a markdown error got in somehow and then breaks all PR jobs, who care nothing about markdown. It sounds like that won't happen.

One improvement would be if a markdown-only change only did markdown testing and nothing else.

It does this already. I verified that yesterday. It's a rather nice experience - 2 mins from PR to green.

@safern
Copy link
Member

safern commented Feb 9, 2021

Yep, the action is configured to only run when md files are changed:

pull_request:
paths:
- "**/*.md"
- ".markdownlint.json"
- ".github/workflows/markdownlint.yml"
- ".github/workflows/markdownlint-problem-matcher.json"

@danmoseley
Copy link
Member

@Youssef1313 do you have any interest in following up to enable more useful rules, like for broken links, malformed tables and headings, etc? I have spent my share of time fixing those things in the past. I would expect we could merge that PR much more quickly than this one as we already have the build step now.

@ViktorHofer
Copy link
Member

@Youssef1313 thanks a lot for your contribution. +1, it would be fantastic to have more rules enabled.

@Youssef1313
Copy link
Member Author

Sure, would be glad to help. May take a while though due to university exams starting next week :/

@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants